Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors breadcrumb handling by introducing a shared BreadcrumbStore contract in @hawk.so/core and updating the JavaScript SDK to use a browser-specific implementation (BrowserBreadcrumbStore) instead of the previous BreadcrumbManager.
Changes:
- Added
BreadcrumbStore(plus related types) to@hawk.so/coreand re-exported them from the core entrypoint. - Refactored the JavaScript SDK breadcrumbs implementation to
BrowserBreadcrumbStoreand updatedCatcherto use the new store API (add/get/clear). - Updated JavaScript tests and types to align with the new names and API surface (and removed the local
breadcrumbs-api.tstype).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/javascript/tests/catcher.user.test.ts | Updates singleton reset/import to BrowserBreadcrumbStore. |
| packages/javascript/tests/catcher.transport.test.ts | Updates singleton reset/import to BrowserBreadcrumbStore. |
| packages/javascript/tests/catcher.test.ts | Updates imports/reset and helper ordering. |
| packages/javascript/tests/catcher.global-handlers.test.ts | Updates singleton reset/import to BrowserBreadcrumbStore. |
| packages/javascript/tests/catcher.context.test.ts | Updates singleton reset/import to BrowserBreadcrumbStore. |
| packages/javascript/tests/catcher.breadcrumbs.test.ts | Updates singleton reset/import to BrowserBreadcrumbStore. |
| packages/javascript/tests/catcher.addons.test.ts | Updates singleton reset/import to BrowserBreadcrumbStore. |
| packages/javascript/tests/breadcrumbs.test.ts | Renames the suite and updates API calls to add/get/clear. |
| packages/javascript/src/types/index.ts | Re-exports breadcrumb store types from @hawk.so/core. |
| packages/javascript/src/types/breadcrumbs-api.ts | Removes the local BreadcrumbsAPI definition in favor of core types. |
| packages/javascript/src/catcher.ts | Switches from BreadcrumbManager to BrowserBreadcrumbStore and updates the public breadcrumbs getter typing. |
| packages/javascript/src/addons/breadcrumbs.ts | Introduces BrowserBreadcrumbStore implementing the core BreadcrumbStore contract and renames browser hint type. |
| packages/core/src/index.ts | Re-exports breadcrumb store types from the core package entrypoint. |
| packages/core/src/breadcrumbs/breadcrumb-store.ts | Adds the new shared breadcrumb store contract and associated types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
don't forget to bump version |
Thx for reminder. |
9890ddb to
afc73fa
Compare
| public init(options: BreadcrumbsOptions = {}): void { | ||
| if (this.isInitialized) { | ||
| log('[BreadcrumbManager] init has already been called; breadcrumb configuration is global and subsequent init options are ignored.', 'warn'); | ||
| log('[BrowserBreadcrumbStore] init has already been called; breadcrumb configuration is global and subsequent init options are ignored.', 'warn'); |
There was a problem hiding this comment.
I don't think users should see such a log. It's better to remove it.
| public init(options: BreadcrumbsOptions = {}): void { | ||
| if (this.isInitialized) { | ||
| log('[BreadcrumbManager] init has already been called; breadcrumb configuration is global and subsequent init options are ignored.', 'warn'); | ||
| log('Breadcrumbs store already initialized', 'warn'); |
There was a problem hiding this comment.
Please, remove this log. Users should not see our debug logs.
If init() is somehow called twice, then we should either:
- throw exception (if it is not normal case)
- hand it silently (if it is ok)
There was a problem hiding this comment.
Log removed.
I guess it's not critical to silently return on second init.
There was a problem hiding this comment.
Please, explain how init can be called twice? Are you sure it is a regular case, not an exception?
| this.breadcrumbStore = BrowserBreadcrumbStore.getInstance(); | ||
| this.breadcrumbStore.init(settings.breadcrumbs ?? {}); | ||
| this.messageProcessors.push(new BreadcrumbsMessageProcessor()); |
There was a problem hiding this comment.
can we initialize BrowserBreadcrumbStore inside the BreadcrumbsMessageProcessor to incapsulate breadcrumbs logic in there and do not pass breadcrumbs to all processors? and get rid of ErrorSnapshot term
There was a problem hiding this comment.
Now we call init inside of BrowserBreadcrumbsMessageProcessor.
However we still need BreadcrumbStore as Catcher field since we have Catcher.breadcrumbs(), so breadcrumbs logic is not fully contained inside message-processor.
acafb78 to
31d54df
Compare
b8c9b5c to
c049bd4
Compare
…ted in catcher event processing pipeline - MessageProcessor interface and MessageHint type - BrowserMessageProcessor, BreadcrumbsMessageProcessor, ConsoleCatcherMessageProcessor, DebugMessageProcessor - replaced inline addon logic with sequential MessageProcessor pipeline
There was a problem hiding this comment.
I dont like the idea to create a "messages" folder just for a single file. What is "messages" in case of core architecture?
There was a problem hiding this comment.
Do we need to create a /breadcrumbs/ directory? It is supposed to see only architecture-like terms on the first level.
There was a problem hiding this comment.
still do not understand what /messages/ are stand for
| * Used to prepare event message before send. | ||
| * May modify original event payload or return null to drop it. | ||
| */ | ||
| messageProcessors?: MessageProcessor<'errors/javascript'>[]; |
There was a problem hiding this comment.
should be documented somewhere, maybe at readme.
Motivation
Part of the broader effort to extract environment-agnostic logic into
@hawk.so/core(#151).BreadcrumbManagerwas a concrete, browser-coupled class living in@hawk.so/javascript—Catcherdepended directly on it, making breadcrumb support impossible to share or override in a non-browser runtime.What changed and why
BreadcrumbStoreis an interface extracted into@hawk.so/core.Catchernow depends on that interface rather than the browser implementation.BrowserBreadcrumbStore(the renamedBreadcrumbManager) is the browser-specific implementation that stays in@hawk.so/javascript.Core defines the contract, concrete catchers wire in the environment-appropriate implementation. A future
NodeCatchercan provide its ownBreadcrumbStorewithout inheriting any browser code.Changes
BreadcrumbStoreinterface added and exportedBreadcrumbManagerrenamed toBrowserBreadcrumbStore, now implementsBreadcrumbStore;Catcherupdated to depend on the interface